-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Using git branch/tag to pull dependencies #147
Conversation
src/commands.rs
Outdated
- **Example from Git with a specified tag:** | ||
soldeer install @openzeppelin-contracts~2.3.0 [email protected]:OpenZeppelin/openzeppelin-contracts.git --tag my-tag | ||
- **Example from Git with a specified branch:** | ||
soldeer install @openzeppelin-contracts~2.3.0 [email protected]:OpenZeppelin/openzeppelin-contracts.git --branch my-branch", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Identation is not the same as the rest of the items above
src/config.rs
Outdated
pub rev: Option<String>, | ||
pub tag: Option<String>, | ||
pub branch: Option<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better represented as an enum, seeing how it's only valid to have one of the three
src/config.rs
Outdated
match &dep.tag { | ||
Some(tag) => { | ||
table.insert( | ||
"version", | ||
value(&dep.version) | ||
.into_value() | ||
.expect("version should be a valid toml value"), | ||
"tag", | ||
value(tag).into_value().expect("tag should be a valid toml value"), | ||
); | ||
table.insert( | ||
"git", | ||
value(&dep.git) | ||
.into_value() | ||
.expect("git URL should be a valid toml value"), | ||
); | ||
table.insert( | ||
"rev", | ||
value(rev).into_value().expect("rev should be a valid toml value"), | ||
); | ||
value(table) | ||
} | ||
None => { | ||
let mut table = InlineTable::new(); | ||
table.insert( | ||
"version", | ||
value(&dep.version) | ||
.into_value() | ||
.expect("version should be a valid toml value"), | ||
); | ||
table.insert( | ||
"git", | ||
value(&dep.git) | ||
.into_value() | ||
.expect("git URL should be a valid toml value"), | ||
); | ||
|
||
value(table) | ||
} | ||
}, | ||
), | ||
None => match &dep.branch { | ||
Some(branch) => { | ||
table.insert( | ||
"branch", | ||
value(branch) | ||
.into_value() | ||
.expect("branch should be a valid toml value"), | ||
); | ||
} | ||
None => match &dep.rev { | ||
Some(rev) => { | ||
table.insert( | ||
"rev", | ||
value(rev) | ||
.into_value() | ||
.expect("rev should be a valid toml value"), | ||
); | ||
} | ||
None => {} | ||
}, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid nested match statements. Much better to match on a tuple: match (&dep.tag, &dep.branch, &dep.rev) { (Some(tag), None, None) => { /* ... */ } }
src/config.rs
Outdated
|
||
let tag = match table.get("tag").map(|v| v.as_str()) { | ||
Some(Some(tag)) => Some(tag.to_string()), | ||
Some(None) => { | ||
return Err(ConfigError::InvalidField { field: "tag".to_string(), dep: name }); | ||
} | ||
None => None, | ||
}; | ||
|
||
let branch = match table.get("branch").map(|v| v.as_str()) { | ||
Some(Some(tag)) => Some(tag.to_string()), | ||
Some(None) => { | ||
return Err(ConfigError::InvalidField { | ||
field: "branch".to_string(), | ||
dep: name, | ||
}); | ||
} | ||
None => None, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is lacking validation here, what if the user sets the rev
and the tag
? We should probably raise an error if more than one is set. See my other comment about an enum type for rev/tag/branch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just wanted to have some sort of hierarchy but maybe yea we should raise an error if more than 1 option is used
src/dependency_downloader.rs
Outdated
let checkout_target = match dependency.rev.clone() { | ||
Some(rev) => Some(rev), | ||
None => match dependency.tag.clone() { | ||
Some(tag) => Some(tag), | ||
None => dependency.branch.clone(), | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid nested match statements, match on a tuple instead.
* fix: rev in git dependency * fix: behavior with no rev
No description provided.